Conversation
Contributor
📝 WalkthroughWalkthroughAdds a trip-invite feature: database migration, TripInvite model and repo, service logic to create and join invites (with code generation, expiry, revocation, and concurrency handling), controller endpoints and routes, OpenAPI docs, and end-to-end tests for invite workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as MembershipController
participant Service as MembershipService
participant InviteRepo as TripInviteRepository
participant MemberRepo as MembershipRepository
participant DB
Client->>Controller: POST /api/v1/trip-invites/{code}/join
Controller->>Service: JoinTripByInviteCode(userID, code)
Service->>InviteRepo: FindByCode(code)
InviteRepo->>DB: SELECT ... WHERE code = ?
DB-->>InviteRepo: invite row / no rows
InviteRepo-->>Service: invite / not found
alt invite invalid/expired/revoked
Service-->>Controller: return 400
else invite valid
Service->>MemberRepo: Find membership by (trip_id,user_id)
MemberRepo->>DB: SELECT membership...
DB-->>MemberRepo: existing membership / no rows
alt already member
MemberRepo-->>Service: existing membership
Service-->>Controller: return 201 existing membership
else not member
Service->>MemberRepo: Create membership (insert)
MemberRepo->>DB: INSERT membership...
DB-->>MemberRepo: success / unique-violation
alt unique-violation (race)
MemberRepo->>DB: SELECT existing membership...
DB-->>MemberRepo: existing membership
MemberRepo-->>Service: existing membership
end
Service-->>Controller: return 201 new membership
end
end
Controller-->>Client: 201 / 400 / 401
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/repository/repository.go (1)
23-34: 🧹 Nitpick | 🔵 TrivialMinor style inconsistency in repository initialization.
Other repositories are initialized with direct struct literals (e.g.,
&userRepository{db: db}), whileTripInviteuses a constructor function. This works correctly but creates a minor inconsistency. Consider aligning with the existing pattern for uniformity, or refactoring all repositories to use constructor functions for consistency.
🤖 Fix all issues with AI agents
In `@backend/internal/controllers/trips.go`:
- Around line 219-224: Don't ignore the error from c.BodyParser when parsing
CreateTripInviteRequest; instead, first check the raw request body via c.Body()
(or len(c.Body()) == 0) and only skip parsing for an empty body, otherwise call
c.BodyParser(&req) and if it returns an error return a 400/validation error
(rather than discarding it). Update the block around
BodyParser/validators.Validate so that for non-empty bodies a parse error is
propagated (e.g., return a fiber 400 with "invalid JSON") and only allow silent
defaulting when the body is truly empty before calling validators.Validate with
ctrl.validator.
In `@backend/internal/migrations/20260205035655_create_trip_invite.sql`:
- Around line 7-14: Remove the redundant non-unique index creation for the
`code` column: delete the `CREATE INDEX idx_trip_invites_code ON
trip_invites(code);` statement (the `code TEXT NOT NULL UNIQUE` already creates
a unique index) and update the down migration so it no longer tries to DROP
`idx_trip_invites_code` (or remove any DROP INDEX for that name) to keep up/down
symmetric; leave the `idx_trip_invites_trip_id` index and the `code` UNIQUE
constraint intact.
In `@backend/internal/models/trip_invites.go`:
- Line 15: The struct field ExpiresAt on the trip invite model uses
contradictory bun tags `nullzero` and `notnull`; remove the `nullzero` tag so
the bun tag becomes `notnull` (and any other intended options like `expires_at`)
to ensure the column is required. Locate the ExpiresAt field in the trip invite
model (symbol ExpiresAt in trip_invites.go), delete `nullzero` from its struct
tag, run tests and any schema/migration checks to ensure the DB column remains
non-nullable.
In `@backend/internal/services/membership.go`:
- Around line 106-122: Extract the duplicated conversion logic into a single
helper, e.g., create a function membershipFromDB(m
*models.MembershipDatabaseResponse) *models.Membership that maps UserID, TripID,
IsAdmin, BudgetMin, BudgetMax, Availability, CreatedAt and UpdatedAt; then
replace the inline conversions in methods that call s.Membership.Find and
AddMember (the blocks that build a models.Membership from
existingMembership/newMembership) to call membershipFromDB(existingMembership)
(or membershipFromDB(newMembership)) instead.
In `@backend/internal/services/trips.go`:
- Around line 265-271: The generateInviteCode function currently uses 6 random
bytes (48 bits); add a brief comment above generateInviteCode documenting the
approximate collision probability (2^48 possible codes ≈ 281 trillion
combinations) and noting the single-retry behavior in the invite-creation path
(the duplicate-check/retry logic that follows) is based on that low probability;
alternatively, if you expect high-volumes, increase entropy by changing the
function to use 8 bytes (64 bits) and update any dependent code/comments
accordingly (keep function name generateInviteCode and ensure callers that
validate duplicates remain compatible).
- Around line 296-306: The retry path after detecting errs.ErrDuplicate must
handle errors from generateInviteCode instead of discarding them: in the block
around s.TripInvite.Create and generateInviteCode, capture the error returned by
generateInviteCode, and if non-nil return it immediately (do not overwrite
invite.Code with an invalid value); only assign invite.Code and call
s.TripInvite.Create again when generateInviteCode succeeds. Ensure the
subsequent err checks still return on failure and reference the existing symbols
s.TripInvite.Create, generateInviteCode, invite.Code, created.
- Around line 308-314: The code currently calls os.Getenv("APP_PUBLIC_URL")
inside CreateTripInvite leading to repeated syscalls; add a field (e.g.,
appPublicURL string) to the service struct in the constructor or a config struct
so the value is read once, initialize s.appPublicURL when the service is
created, and replace the getenv call in CreateTripInvite with s.appPublicURL;
ensure you still trim trailing slashes and build the joinURL using created.Code
(and update any tests to inject the appPublicURL via the service
constructor/config).
In `@backend/internal/tests/invites_test.go`:
- Around line 95-129: The test inserts a TripInvite directly using
db.NewInsert() which can leave state behind; add a t.Cleanup callback
immediately after the insert that deletes the inserted record (use
db.NewDelete().Model for models.TripInvite and filter by invite.ID) to ensure
isolation and prevent flakiness; place the cleanup right after the call that
sets invite (i.e., after NewInsert().Model(invite).Exec) so the inserted invite
is removed after the test finishes.
Comment on lines
+7
to
+14
| code TEXT NOT NULL UNIQUE, | ||
| expires_at TIMESTAMP WITH TIME ZONE NOT NULL, | ||
| is_revoked BOOLEAN NOT NULL DEFAULT false, | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT now() | ||
| ); | ||
|
|
||
| CREATE INDEX idx_trip_invites_trip_id ON trip_invites(trip_id); | ||
| CREATE INDEX idx_trip_invites_code ON trip_invites(code); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Index on code is redundant with UNIQUE constraint.
Line 7 declares code TEXT NOT NULL UNIQUE, which automatically creates a unique index. Line 14 creates an additional index idx_trip_invites_code on the same column. This duplicate index wastes storage and slows writes.
Remove redundant index
CREATE INDEX idx_trip_invites_trip_id ON trip_invites(trip_id);
-CREATE INDEX idx_trip_invites_code ON trip_invites(code);
CREATE INDEX idx_trip_invites_created_by ON trip_invites(created_by);Also update the down migration:
DROP INDEX IF EXISTS idx_trip_invites_created_by;
-DROP INDEX IF EXISTS idx_trip_invites_code;
DROP INDEX IF EXISTS idx_trip_invites_trip_id;🤖 Prompt for AI Agents
In `@backend/internal/migrations/20260205035655_create_trip_invite.sql` around
lines 7 - 14, Remove the redundant non-unique index creation for the `code`
column: delete the `CREATE INDEX idx_trip_invites_code ON trip_invites(code);`
statement (the `code TEXT NOT NULL UNIQUE` already creates a unique index) and
update the down migration so it no longer tries to DROP `idx_trip_invites_code`
(or remove any DROP INDEX for that name) to keep up/down symmetric; leave the
`idx_trip_invites_trip_id` index and the `code` UNIQUE constraint intact.
| TripID uuid.UUID `bun:"trip_id,type:uuid,notnull" json:"trip_id"` | ||
| CreatedBy uuid.UUID `bun:"created_by,type:uuid,notnull" json:"created_by"` | ||
| Code string `bun:"code,notnull" json:"code"` | ||
| ExpiresAt time.Time `bun:"expires_at,nullzero,notnull" json:"expires_at"` |
Contributor
There was a problem hiding this comment.
Contradictory bun tags: nullzero with notnull on expires_at.
The nullzero tag instructs bun to convert Go's zero time.Time to SQL NULL, while notnull declares the column cannot be NULL. If ExpiresAt is ever unset (zero value), the insert will fail at the database level. The service currently always sets this field, but this tag combination is error-prone for future maintainers.
Consider removing nullzero since ExpiresAt is required:
Proposed fix
- ExpiresAt time.Time `bun:"expires_at,nullzero,notnull" json:"expires_at"`
+ ExpiresAt time.Time `bun:"expires_at,notnull" json:"expires_at"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ExpiresAt time.Time `bun:"expires_at,nullzero,notnull" json:"expires_at"` | |
| ExpiresAt time.Time `bun:"expires_at,notnull" json:"expires_at"` |
🤖 Prompt for AI Agents
In `@backend/internal/models/trip_invites.go` at line 15, The struct field
ExpiresAt on the trip invite model uses contradictory bun tags `nullzero` and
`notnull`; remove the `nullzero` tag so the bun tag becomes `notnull` (and any
other intended options like `expires_at`) to ensure the column is required.
Locate the ExpiresAt field in the trip invite model (symbol ExpiresAt in
trip_invites.go), delete `nullzero` from its struct tag, run tests and any
schema/migration checks to ensure the DB column remains non-nullable.
Comment on lines
+106
to
+122
| // If already a member, return existing membership. | ||
| existingMembership, err := s.Membership.Find(ctx, userID, invite.TripID) | ||
| if err == nil { | ||
| return &models.Membership{ | ||
| UserID: existingMembership.UserID, | ||
| TripID: existingMembership.TripID, | ||
| IsAdmin: existingMembership.IsAdmin, | ||
| BudgetMin: existingMembership.BudgetMin, | ||
| BudgetMax: existingMembership.BudgetMax, | ||
| Availability: existingMembership.Availability, | ||
| CreatedAt: existingMembership.CreatedAt, | ||
| UpdatedAt: existingMembership.UpdatedAt, | ||
| }, nil | ||
| } | ||
| if !errors.Is(err, errs.ErrNotFound) { | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting membership conversion to a helper.
The code that converts MembershipDatabaseResponse to Membership (lines 109-118, 143-152) is duplicated here and also appears in AddMember (lines 62-71). A helper method would reduce repetition.
Optional helper extraction
func toMembership(m *models.MembershipDatabaseResponse) *models.Membership {
return &models.Membership{
UserID: m.UserID,
TripID: m.TripID,
IsAdmin: m.IsAdmin,
BudgetMin: m.BudgetMin,
BudgetMax: m.BudgetMax,
Availability: m.Availability,
CreatedAt: m.CreatedAt,
UpdatedAt: m.UpdatedAt,
}
}Also applies to: 135-155
🤖 Prompt for AI Agents
In `@backend/internal/services/membership.go` around lines 106 - 122, Extract the
duplicated conversion logic into a single helper, e.g., create a function
membershipFromDB(m *models.MembershipDatabaseResponse) *models.Membership that
maps UserID, TripID, IsAdmin, BudgetMin, BudgetMax, Availability, CreatedAt and
UpdatedAt; then replace the inline conversions in methods that call
s.Membership.Find and AddMember (the blocks that build a models.Membership from
existingMembership/newMembership) to call membershipFromDB(existingMembership)
(or membershipFromDB(newMembership)) instead.
Comment on lines
+308
to
+314
| var joinURL *string | ||
| baseURL := os.Getenv("APP_PUBLIC_URL") | ||
| if baseURL != "" { | ||
| trimmed := strings.TrimRight(baseURL, "/") | ||
| u := trimmed + "/invites/" + created.Code | ||
| joinURL = &u | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid reading environment variable on every request.
os.Getenv("APP_PUBLIC_URL") is called on each invite creation. Inject this value through the service constructor or a config struct to avoid repeated syscalls and improve testability.
Proposed approach
type TripService struct {
*repository.Repository
fileService FileServiceInterface
publisher realtime.EventPublisher
+ appPublicURL string
}
-func NewTripService(repo *repository.Repository, fileService FileServiceInterface, publisher realtime.EventPublisher) TripServiceInterface {
+func NewTripService(repo *repository.Repository, fileService FileServiceInterface, publisher realtime.EventPublisher, appPublicURL string) TripServiceInterface {
return &TripService{
Repository: repo,
fileService: fileService,
publisher: publisher,
+ appPublicURL: appPublicURL,
}
}Then use s.appPublicURL instead of os.Getenv("APP_PUBLIC_URL") in CreateTripInvite.
🤖 Prompt for AI Agents
In `@backend/internal/services/trips.go` around lines 308 - 314, The code
currently calls os.Getenv("APP_PUBLIC_URL") inside CreateTripInvite leading to
repeated syscalls; add a field (e.g., appPublicURL string) to the service struct
in the constructor or a config struct so the value is read once, initialize
s.appPublicURL when the service is created, and replace the getenv call in
CreateTripInvite with s.appPublicURL; ensure you still trim trailing slashes and
build the joinURL using created.Code (and update any tests to inject the
appPublicURL via the service constructor/config).
Comment on lines
+95
to
+129
| t.Run("expired invite returns 400", func(t *testing.T) { | ||
| app := fakes.GetSharedTestApp() | ||
| db := fakes.GetSharedDB() | ||
|
|
||
| owner := createUser(t, app) | ||
| trip := createTrip(t, app, owner) | ||
|
|
||
| code := "expired-" + uuid.NewString() | ||
| expired := time.Now().UTC().Add(-1 * time.Hour) | ||
|
|
||
| invite := &models.TripInvite{ | ||
| ID: uuid.New(), | ||
| TripID: uuid.MustParse(trip), | ||
| CreatedBy: uuid.MustParse(owner), | ||
| Code: code, | ||
| ExpiresAt: expired, | ||
| IsRevoked: false, | ||
| CreatedAt: time.Now().UTC(), | ||
| } | ||
|
|
||
| _, err := db.NewInsert().Model(invite).Exec(context.Background()) | ||
| require.NoError(t, err) | ||
|
|
||
| user := createUser(t, app) | ||
|
|
||
| testkit.New(t). | ||
| Request(testkit.Request{ | ||
| App: app, | ||
| Route: fmt.Sprintf("/api/v1/trip-invites/%s/join", code), | ||
| Method: testkit.POST, | ||
| UserID: &user, | ||
| }). | ||
| AssertStatus(http.StatusBadRequest). | ||
| AssertField("message", "invite link has expired") | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider test data cleanup for isolation.
Tests that directly insert data via db.NewInsert() (lines 115 and 151) may leave records in the shared test database. If other tests depend on specific invite counts or codes, this could cause flaky behavior. Consider adding cleanup in a t.Cleanup() callback or using unique code prefixes with a cleanup sweep.
Proposed cleanup pattern
// After inserting the invite
t.Cleanup(func() {
_, _ = db.NewDelete().Model((*models.TripInvite)(nil)).Where("id = ?", invite.ID).Exec(context.Background())
})🤖 Prompt for AI Agents
In `@backend/internal/tests/invites_test.go` around lines 95 - 129, The test
inserts a TripInvite directly using db.NewInsert() which can leave state behind;
add a t.Cleanup callback immediately after the insert that deletes the inserted
record (use db.NewDelete().Model for models.TripInvite and filter by invite.ID)
to ensure isolation and prevent flakiness; place the cleanup right after the
call that sets invite (i.e., after NewInsert().Model(invite).Exec) so the
inserted invite is removed after the test finishes.
| CreatedAt time.Time `bun:"created_at,nullzero" json:"created_at"` | ||
| } | ||
|
|
||
| func (TripInvite) TableName() string { |
Contributor
There was a problem hiding this comment.
can we remove this, i don't think it's being used anywhere
Contributor
Author
There was a problem hiding this comment.
Which thing are you referring to? the created at or tablename()
in-mai-space
requested changes
Feb 11, 2026
Contributor
in-mai-space
left a comment
in-mai-space
left a comment
There was a problem hiding this comment.
GREAT JOB @jleung40 🔥 few small nits and should be good to go!
aahiltn
reviewed
Feb 12, 2026
| "context" | ||
|
|
||
| v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" | ||
| "github.com/aws/aws-sdk-go-v2/aws/signer/v4" |
Contributor
There was a problem hiding this comment.
damn im not sure why this keeps popping up and disappearing in some prs. must be local linter settings. (no change needed)
Contributor
|
Good call on the endpoint structuring! Sound logic to me. Just fix those nits, and you'll be good to merge my goat J |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Created the POST /trips/:id/invites and POST /trip-invites/:code/join. Notably, this differs from the original ticket instructions of creating a POST /trips/invites/:code/join endpoint. This is made as /trip-invites, since /trips/invites/:code/join was causing fiber to pick up the /invites part of the endpoint as an ID. Anyways, for the invite link url system to work, an env variable for APP_PUBLIC_URL needs to be set, as it's used in trips.go
How has this been tested?
Added tests in invite_tests.go
Checklist
User-Visible Changes
/trips/{tripID}/invitescreates shareable invites with optional custom expiration; POST/trip-invites/{code}/joinallows users to join trips via invite codes.APP_PUBLIC_URLenvironment variable is configured.Changes by Category
Features
Infra
trip_invitesdatabase table with indexes on trip_id, code, and created_by; cascading deletes from trips and users.TripInviteRepositorylayer implementing Create, FindByID, and FindByCode operations.20260205035655_create_trip_invite.sql.Testing
invites_test.gocovering invite creation, successful join, invalid/expired/revoked invites, and authentication.Documentation
Author Contribution
Sequence Flow: Invite Join Operation